Skip to content

Fix GH-12727: NumberFormatter constructor throws an exception on #12868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Dec 4, 2023

invalid locale.

Also re-establishing exception throwing on IntlDateFormatter constructor overwritten by accident most likely so postponing it for next major release.

@devnexen devnexen force-pushed the ext_intl_locale_except branch 2 times, most recently from a5840e8 to 4fd8ae4 Compare December 4, 2023 22:29
@devnexen devnexen marked this pull request as ready for review December 4, 2023 23:47
@devnexen devnexen requested a review from kocsismate December 4, 2023 23:47
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sensible to me

@@ -63,6 +64,11 @@ static int numfmt_ctor(INTERNAL_FUNCTION_PARAMETERS, zend_error_handling *error_
locale = intl_locale_get_default();
}

if (strlen(uloc_getISO3Language(locale)) == 0) {
zend_argument_value_error(1, "is invalid");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice to show what the locale is to say that it is invalid. So maybe something like

Suggested change
zend_argument_value_error(1, "is invalid");
zend_argument_value_error(1, " \"%s\" is invalid", locale);

@devnexen devnexen force-pushed the ext_intl_locale_except branch from 908d199 to cfef555 Compare December 5, 2023 07:08
Comment on lines 116 to 117
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR, "datefmt_create: invalid locale", 0);
return FAILURE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR, "datefmt_create: invalid locale", 0);
return FAILURE;
zend_argument_value_error(1, "\"%s\" is invalid", locale);
return FAILURE;

Apply the OO error style to procedural?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep agreed

invalid locale.

Also re-establishing exception throwing on IntlDateFormatter constructor
overwritten by accident most likely so postponing it for next major
release.
@devnexen devnexen force-pushed the ext_intl_locale_except branch from cfef555 to a7b7bba Compare December 6, 2023 05:33
@devnexen devnexen closed this in 683e787 Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants